Fix providing progressToken with request #405
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where progress tokens were not being properly attached to request parameters when a progress handler is provided. The fix ensures that when onProgress is specified in RequestOptions, the request's _meta field is correctly updated with the progressToken.
Key Changes:
- Modified request handling to inject
progressTokeninto request metadata when progress handling is enabled - Added comprehensive test coverage for progress token scenarios
- Improved variable naming and error handling in the Protocol class
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/Protocol.kt | Core fix: adds logic to inject progressToken into request params when onProgress is provided; improves variable naming (messageId → jsonRpcRequestId) and error handling (Error → IllegalStateException) |
| kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/ProtocolTest.kt | Adds comprehensive test coverage for progress token handling including cases for preserving existing meta, creating meta when absent, and handling null params |
| kotlin-sdk-core/build.gradle.kts | Adds kotlinx-coroutines-test dependency required for the new test infrastructure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kpavlov
left a comment
kpavlov
left a comment
There was a problem hiding this comment.
Thank you for the fix.
I have a couple of comments, but it can be addressed later
| override fun assertRequestHandlerCapability(method: Method) {} | ||
| } | ||
|
|
||
| private class RecordingTransport : Transport { |
There was a problem hiding this comment.
good stuff, let's extract it
| private var onMessageCallback: (suspend (JSONRPCMessage) -> Unit)? = null | ||
| private var onCloseCallback: (() -> Unit)? = null | ||
|
|
||
| override suspend fun start() {} |
There was a problem hiding this comment.
it make sense to add debug logging here
| } | ||
| } | ||
|
|
||
| private class TestProtocol : Protocol(null) { |
There was a problem hiding this comment.
good stuff, let's extract it
kotlin-sdk-core/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/ProtocolTest.kt
Outdated
Show resolved
Hide resolved
…ty and null safety
devcrocod
force-pushed
the
devcrocod/fix-protocol
branch
from
83ddf82 to
a130902
Compare
Motivation and Context
fix #220
How Has This Been Tested?
locally
Breaking Changes
None
Types of changes
Checklist